Review requested:

• @nodejs/http2
• @nodejs/net

Codecov Report

❌ Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (debe2ed) to head (4ad1d1f).
⚠️ Report is 93 commits behind head on main.

@@            Coverage Diff             @@
##             main   #63414      +/-   ##
==========================================
+ Coverage   90.05%   90.30%   +0.25%     
==========================================
  Files         714      730      +16     
  Lines      225742   234164    +8422     
  Branches    42727    43913    +1186     
==========================================
+ Hits       203285   211471    +8186     
- Misses      14234    14418     +184     
- Partials     8223     8275      +52     

This changes the behaviour exclusively for this one case (where the underlying socket has actually been destroyed). I think there's quite a few other cases where you'll get very similar symptoms (e.g. GOAWAY, any other non-socket session issues that trigger session teardown) where you'll still hit the same event order, and so Undici would still hit the same issue even with this fix.

More generally, having the event ordering branch dependingh on error scenarios seems like it's going to be confusing - it'd be better to have a single ordering all the time. And the ordering is a bit surprising, I would normally expect streams within to close before sessions containing them... I worry that flipping the session/stream closure order will just expose some other race condition in the other direction somehow, since this change would now mean that session close would fire while every stream appeared to be open as normal.

Can we fix this in a cleaner way? What if we continue closing the streams first, like today (more intuitive imo, keeps single consistent ordering of events) but we emit all errors like this (any problems other than param validation) async instead of sync? Seems safe-ish, since async failure is clearly already possible and has to be handled anyway. There's similar other cases too already: if you call session.request while the session is still connecting, the request flow and any potential errors are deferred to async in all cases today.

That'd solve Undici's issue: you could still just listen to the events, and nothing will fail synchronously if you retry a new stream when one closes - it's just that the 2nd request will later emit 'error' (as it can in other cases today anyway).

That would also match HTTP/1 better. The code there explicitly pushes all initial sync connection issues to next tick:

Thanks, I changed direction here based on the review.

Instead of reordering session/stream teardown, this now keeps the existing shutdown order and fixes the race by making ClientHttp2Session.request() defer invalid-session state errors to the returned stream.

So if user code retries from a stream 'close' callback while the cached session is already stale, it no longer throws synchronously with ERR_HTTP2_INVALID_SESSION. It returns a stream that emits ERR_HTTP2_INVALID_SESSION or ERR_HTTP2_GOAWAY_SESSION asynchronously, which gives session lifecycle handlers a chance to clear cached state first.

Updated in this branch:

• reverted the close-event ordering change in closeSession()
• changed request() to keep argument validation synchronous, but defer session-state failures asynchronously
• updated tests to cover the teardown/retry case and the destroy path
• updated the http2 docs for the new request() behavior

Validated locally with:

• make -j$(nproc)
• make lint-js LINT_JS_TARGETS='lib/internal/http2/core.js test/parallel/test-http2-client-destroy.js test/parallel/test-http2-client-session-close-before-stream-close.js'
• make lint-md LINT_MD_FILES='doc/api/http2.md'
• python3 tools/test.py test/parallel/test-http2-client-destroy.js test/parallel/test-http2-goaway-delayed-request.js test/parallel/test-http2-client-session-close-before-stream-close.js test/parallel/test-http2-server-shutdown-redundant.js

Follow-up for the Darwin CI failures: I pushed a small test-only adjustment.

The implementation change looks fine, but test-http2-client-session-close-before-stream-close.js was too strict about the exact teardown state observed in the first stream 'close' callback. On the shared-library Darwin jobs, session.closed is already true there, but session.destroyed is not always observable yet.

The test now checks the behavior we actually care about:

• session.request() does not throw synchronously from the stream 'close' callback
• the returned stream reports the failure asynchronously
• cached session state has been cleared before that async error is handled

It now accepts either ERR_HTTP2_INVALID_SESSION or ERR_HTTP2_GOAWAY_SESSION for that callback timing.